-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: auto-approve checkbox now expands menu when no options selected #7549
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- When no auto-approve options are selected, clicking the checkbox area will expand the menu to show available options - This improves UX by making it clearer how to enable auto-approval features - Updated tests to verify the new behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code because apparently I trust no one, not even myself.
| onClick={toggleExpanded}> | ||
| <div onClick={(e) => e.stopPropagation()}> | ||
| <div | ||
| onClick={(e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this with keyboard navigation? The click handler on the div wrapper could potentially interfere with keyboard accessibility or screen reader interactions. It might be worth verifying that the checkbox remains fully accessible.
| if (!hasEnabledOptions) { | ||
| setIsExpanded(true) | ||
| } | ||
| }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested click handlers with stopPropagation() create a somewhat complex event flow. Could we simplify this by having a single click handler that checks both conditions? Something like:
| }}> | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| // If no options are selected, clicking the checkbox area should expand the menu | |
| if (!hasEnabledOptions) { | |
| setIsExpanded(true) | |
| } else { | |
| // Handle normal checkbox toggle | |
| const newValue = !(autoApprovalEnabled ?? false) | |
| setAutoApprovalEnabled(newValue) | |
| vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue }) | |
| } | |
| }}> |
| const masterCheckbox = screen.getByRole("checkbox") | ||
| fireEvent.click(masterCheckbox) | ||
| const checkboxContainer = masterCheckbox.parentElement | ||
| fireEvent.click(checkboxContainer!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using parentElement without null checking could be fragile. Consider adding a null check or using a more specific selector:
| fireEvent.click(checkboxContainer!) | |
| const checkboxContainer = masterCheckbox.parentElement | |
| if (!checkboxContainer) { | |
| throw new Error('Checkbox container not found') | |
| } | |
| fireEvent.click(checkboxContainer) |
| vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue }) | ||
| } | ||
| // If no options enabled, do nothing | ||
| // If no options enabled, the onClick handler above will expand the menu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment might be clearer if it mentioned this is the fallback behavior:
| // If no options enabled, the onClick handler above will expand the menu | |
| // If no options enabled, the onClick handler above will expand the menu | |
| // (since onChange won't fire for a disabled checkbox) |
|
This doesn't work currently, tried to fix it but it is kinda janky, we might want to think of a different solution for this. |
|
Closing in favor of #7421 |
Summary
This PR improves the UX of the auto-approve checkbox by making it expand the options menu when clicked while no options are selected.
Problem
Previously, when no auto-approve options were selected, the checkbox was disabled and clicking it did nothing. This was confusing for users who expected clicking the checkbox to show them the available options.
Solution
Changes
AutoApproveMenu.tsxto add click handler that expands menu when no options selectedTesting
Context
This change was requested via Slack: "I think the checkbox should probably pop open the list if nothing is selected - it's a little confusing otherwise"
Fixes the UX confusion by making the interface more intuitive.
Important
Improves UX by expanding the auto-approve options menu when the checkbox is clicked with no options selected in
AutoApproveMenu.tsx.AutoApproveMenu.tsx: Checkbox now expands options menu when clicked with no options selected.AutoApproveMenu.spec.tsx: Updated tests to verify menu expansion behavior when no options are selected.This description was created by
for 3c480ca. You can customize this summary. It will automatically update as commits are pushed.